-
Notifications
You must be signed in to change notification settings - Fork 7
Adding requirements on major version of XRootD (software-4137) #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor changes requested; see comments.
BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) | ||
BuildRequires: xrootd-libs-devel xerces-c-devel pcre-devel | ||
|
||
%define xrootd_current_major 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow this to be overridden via an RPM macro specified at build time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this would play with osg-build, could we leave that for the next time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 0%{?osg}
%define xrootd_current_major 4
%endif
This should be sufficient (assuming this looks right to @matyasselmeci)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that the idea was to be able to set the version number at build time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what @brianhlin wrote will work. I'm not sure what you mean by "RPM macro specified at build time".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that the idea was to be able to set the version number at build time
This gets us halfway there: if someone wants to build this outside of an OSG build environment, they'll have to specify xrootd_current_major
. @matyasselmeci is there a way to specify xrootd_current_major
as an argument to osg-build
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing arbitrary --define options to rpmbuild and mock commands is doable but not currently implemented. You can currently work around it by defining those in your ~/.rpmmacros
file.
Passing --defines to Koji builds is impossible.
I strongly caution against depending on users to define specific macros via the command line, since it harms reproducibility -- you have to remember what arguments you passed to rpmbuild. If you want to change something, edit the spec file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would just be nice to specify a single spec file that can support builds against XRootD 4 and XRootD 5. This will have to do, though.
|
||
%cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo . | ||
cd %{name}-%{version} | ||
%cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_INSTALL_LIBDIR=%{_lib} . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these lines necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, this changes were in the osg svn repo for god knows how long and there is no mention of them in the changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to get rid of the cd %{name}-%{version}
by changing the %setup%.
-DCMAKE_INSTALL_LIBDIR=%{_lib}` is needed so that files get installed into /usr/lib64 instead of /usr/lib.
NOTE: some of these fixes may already be implemented in the pending pull requests to the original repository. However, it contains a mix of things, so I chose to do an own fork for the moment. opensciencegrid#3 - enable correct library location in .../lib64/ - unpack tarball into correct version dir
This adds the requirements on the major version of XRootD.
I used as a base the spec file currently used in OSG which adds few other minor changes